-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
implement pthread_cancel -- DO NOT MERGE, ARCHIVE ONLY. #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase and in your PR include changes to pthread_cancel()
related only.
I did not fully review pthread_cancel()
, let's get the rest out of the way first.
...s_POSIX_with_actor_Windows_Simulator/lib/FreeRTOS-Plus-POSIX/source/FreeRTOS_POSIX_pthread.c
Outdated
Show resolved
Hide resolved
FreeRTOS-Plus/Demo/FreeRTOS_Plus_POSIX_with_actor_Windows_Simulator/posix_demo.c
Outdated
Show resolved
Hide resolved
...s_POSIX_with_actor_Windows_Simulator/lib/FreeRTOS-Plus-POSIX/source/FreeRTOS_POSIX_pthread.c
Outdated
Show resolved
Hide resolved
I'm aware we haven't have everything fully setup in this repo. Please include some detail about what level of test you have done (e.g. compiled || run on which platform || etc.). (Funny I'm commenting on test, given we can't accept test cases yet in this repo.) |
…RTOS-POSIX # Conflicts: # FreeRTOS-Plus/Demo/FreeRTOS_Plus_POSIX_with_actor_Windows_Simulator/FreeRTOSConfig.h # FreeRTOS-Plus/Demo/FreeRTOS_Plus_POSIX_with_actor_Windows_Simulator/posix_demo.c # FreeRTOS-Plus/Demo/FreeRTOS_Plus_POSIX_with_actor_Windows_Simulator/posix_demo.h
|
||
/* Threads may already be successfully ended elsewhere and pending full deletion in idle task. */ | ||
/* Note the the xJoinBarrier is a binary sem, so no new queue items are made once its already given */ | ||
if( pxThread && pxThread->xTaskHandle && ( eDeleted != eTaskGetState( pxThread->xTaskHandle ) ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finished my meditation, and the implementation proposed here I believe is incorrect. I'm leaving this PR open mainly for documentation purpose. I'm commenting my reasoning below in detail.
The implementation here suspends/deletes the task without checking for "cancelability state". The detailed description of "thread cancellation" and "cancellation points" are described in POSIX Threads chapter https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_09_05.
Specifically:
Cancellation is controlled by the cancellation control functions. Each thread maintains its own cancelability state. Cancellation may only occur at cancellation points or when the thread is asynchronously cancelable.
(A direct counter example to the implementation in this PR is -- what if the thread being cancelled is holding a mutex? If it's detached, we can't just delete it... )
When talking about FreeRTOS+POSIX, mainly as an adapter layer to FreeRTOS APIs, we need to investigate a couple of things:
- How to track per thread cencelability state. (See TCB definition, and consider how to add the state in either POSIX layer or kernel layer.)
- Thread state machine and transit condition, see what thread cancellation means in each state. (And see how cancellation point apply in such context.)
- If we are talking about kernel changes, please have performance reasonings ready so that we could pick an implementation.
The quick summary is thread cancelation cannot be done with a single function as proposed in this PR.
I don't expect the test functions to make it through, but to be used for reviewer verification.
This is per aws/amazon-freertos#587